uefi: Refactor PciRootBridgeIo::enumerate() with tree-topology information #1830

seijikun · 2025-11-21T22:14:45Z

Refactored return type from standard BTreeSet<PciIoAddress> to custom PciTree struct with tree topology information
Removed special FullPciIoAddress type, since segment number is fixed per PciRoot
During enumeration, skip branches we have already seen
During enumeration, collect tree topology information (which child bus linked from where)
Add complicated pci structure in integration test vm
Print child busses for every device entry in integration test

Previously, we iterated over the bus range from the ACPI descriptor. I changed this in my last MR to only visit the first in the range. I changed this back to iterate over all of the entries, but skip the ones we have seen previously - now that that's easily possible. I decided that better safe than sorry is the best way. The iteration over all child busses already saved our ass once - let's not willingly take this safety net away. Sorry for the back and forth here.

The plan for device paths is now to add something like: PciTree::device_path(&self, addr: PciIoAddress) -> PoolDevicePath.

Checklist

Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
Update the changelog (if necessary)

phip1611

Thanks for working on this! My main concern is my lack of domain knowledge of PCIe. Please elaborate a little more to help me understand what's going on :)

uefi/src/proto/pci/enumeration.rs

phip1611 · 2025-11-22T09:21:28Z

xtask/src/qemu.rs

+    // test is more interesting.
+    cmd.args([
+        "-device",
+        "ioh3420,id=root_port1,bus=pcie.0",


I'm not that familiar with PCI. Could you please elaborate a little?

why adding a root port? Is this equivalent to adding a new PCI bus?

what is x3130-upstream and why are you using it here?

what is x3130-downstream and why are you using it here?

virtio-scsi-pci is just a random PCI device without serving a functionality here except for appearing on the PCIe bus?

why are you specifying chassis=9|10?

Much of this is qemu specific syntax:
ioh3420 reserves a port in pcie.0 - the pcie root port, also called pcie root complex. I imagine this like soldering a PCIe socket on your mainboard, where you can plug stuff in.
Into this socket, we then plug a x3130-upstream. That's a PCIe switch.
x3130-downstream is the other side of the swich, where you can plug child devices in.
I have no idea what chassis is, the way I understood it, it can be any number as long as they are unique. All bridges/switches have this obligatory parameter.
The scsi devices are just for testing, like you said.

Difference PCIe switches and PCIe bridges:
Both allocate a new subordinate bus. Both allow connecting devices to them, which they "pass through" to the port where they are connected upstream. The key difference is, that for you to pass 2 lanes through a bridge, it needs to be connected with 2 lanes to its upstream. Whereas PCIe switches can do switching. They can tunnel y downstream lanes through x upstream lanes - sacrificing speed if y > x (like with network switches). They are rather seldom, which is why I wanted to test it.

https://blogs.oracle.com/linux/a-study-of-the-linux-kernel-pci-subsystem-with-qemu

phip1611 · 2025-11-22T09:23:49Z

uefi-test-runner/src/proto/pci/root_bridge.rs

-                "PCI Device: [{bus}, {dev}, {fun}]: vendor={vendor_id:04X}, device={device_id:04X}, class={class_code:02X}, subclass={subclass_code:02X}"
+                "PCI Device: [{bus:02x}, {dev:02x}, {fun:02x}]: vendor={vendor_id:04X}, device={device_id:04X}, class={class_code:02X}, subclass={subclass_code:02X}"
            );
+            for child_bus in pci_tree.iter_subsequent_busses_for(addr) {


instead of just iterating, would it make sense to explictely check for the expected PCIe devices?

The bus numbers are dynamically allocated and thus might vary.
But I've done something like you suggest in the follow-up MR here, based on device paths: 582714a

xtask/src/qemu.rs

…ation - Refactored return type from standard BTreeSet to custom PciTree struct - Removed special FullPciIoAddress type, since segment number is PciRoot dependent - During enumeration, skip branches we have already seen - During enumeration, collect tree topology information (which child bus linked from where) - Add complicated pci structure in integration test vm - Print child busses for every device entry in integration test

phip1611

We're almost there! Just a few nits

phip1611 · 2025-11-23T07:38:44Z

uefi/src/proto/pci/enumeration.rs

+        }
+    }
+
+    pub(crate) fn push_bus(&self, bus: u8) -> bool {


Further, should this be called can_bush_bridge to be consistent with the other method`

Suggested change

pub(crate) fn push_bus(&self, bus: u8) -> bool {

pub(crate) fn can_push_bus(&self, bus: u8) -> bool {

phip1611 · 2025-11-23T07:39:39Z

uefi/src/proto/pci/enumeration.rs

+    pub fn iter_subsequent_busses_for(&self, addr: PciIoAddress) -> impl Iterator<Item = u8> {
+        self.bus_anchors
+            .iter()
+            .filter(move |&(_, parent)| *parent == addr)


Do you really need move here? PciIoAddress is Copy

phip1611 · 2025-11-23T07:40:46Z

uefi/src/proto/pci/enumeration.rs

+
+    /// Iterate over all subsequent busses below the given `addr`.
+    /// This yields 0 results if `addr` doesn't point to a PCI bridge.
+    pub fn iter_subsequent_busses_for(&self, addr: PciIoAddress) -> impl Iterator<Item = u8> {


Bz convention I think all functions returning an iterator end with iter

Suggested change

pub fn iter_subsequent_busses_for(&self, addr: PciIoAddress) -> impl Iterator<Item = u8> {

pub fn subsequent_busses_filtered_iter(&self, addr: PciIoAddress) -> impl Iterator<Item = u8> {

phip1611 · 2025-11-23T07:43:37Z

uefi/src/proto/pci/enumeration.rs

+        self.devices.insert(addr);
+    }
+
+    pub(crate) fn push_bridge(&mut self, addr: PciIoAddress, child_bus: u8) -> bool {


Suggested change

pub(crate) fn push_bridge(&mut self, addr: PciIoAddress, child_bus: u8) -> bool {

/// Pushes a new bridge into the topology.

///

/// Returns `false` if the bus is already in the topology and `true`

/// if the bridge was added to the topology.

pub(crate) fn push_bridge(&mut self, addr: PciIoAddress, child_bus: u8) -> bool {

phip1611 · 2025-11-23T07:44:30Z

uefi/src/proto/pci/enumeration.rs

+
+    /// Get the segment number of this PCI tree.
+    #[must_use]
+    pub const fn segment_nr(&self) -> u32 {


Suggested change

pub const fn segment_nr(&self) -> u32 {

pub const fn segment(&self) -> u32 {

phip1611 · 2025-11-23T07:47:08Z

xtask/src/qemu.rs

+    //            +-1f.2
+    //            \-1f.3
+    // In the diagram, `0000:03:00.0` and `0000:04:00.0` are just dummy SCSI devices
+    // connected below the PCIe switch, to correct recursion in the bus enumeration implementation.


This sentence sounds broken? Maybe this

Suggested change

// connected below the PCIe switch, to correct recursion in the bus enumeration implementation.

// connected below the PCIe switch, to test correct recursion in the bus enumeration implementation.

seijikun force-pushed the mr-pci-enumerate3 branch 5 times, most recently from 2a46de0 to fbed494 Compare November 21, 2025 22:26

seijikun mentioned this pull request Nov 22, 2025

uefi: Add device path generation for discovered devices in a PciTree #1831

Draft

2 tasks

phip1611 requested changes Nov 22, 2025

View reviewed changes

phip1611 reviewed Nov 22, 2025

View reviewed changes

xtask/src/qemu.rs Show resolved Hide resolved

seijikun force-pushed the mr-pci-enumerate3 branch from fbed494 to 602c776 Compare November 22, 2025 11:24

seijikun force-pushed the mr-pci-enumerate3 branch from 602c776 to a82b06c Compare November 22, 2025 11:26

seijikun requested a review from phip1611 November 22, 2025 12:08

phip1611 requested changes Nov 23, 2025

View reviewed changes

	pub(crate) fn push_bus(&self, bus: u8) -> bool {
	pub(crate) fn can_push_bus(&self, bus: u8) -> bool {

	pub fn iter_subsequent_busses_for(&self, addr: PciIoAddress) -> impl Iterator<Item = u8> {
	pub fn subsequent_busses_filtered_iter(&self, addr: PciIoAddress) -> impl Iterator<Item = u8> {

-    pub(crate) fn push_bridge(&mut self, addr: PciIoAddress, child_bus: u8) -> bool {
+    /// Pushes a new bridge into the topology.
+    ///
+    /// Returns `false` if the bus is already in the topology and `true`
+    /// if the bridge was added to the topology.
+    pub(crate) fn push_bridge(&mut self, addr: PciIoAddress, child_bus: u8) -> bool {

	pub const fn segment_nr(&self) -> u32 {
	pub const fn segment(&self) -> u32 {

	// connected below the PCIe switch, to correct recursion in the bus enumeration implementation.
	// connected below the PCIe switch, to test correct recursion in the bus enumeration implementation.

Uh oh!

uefi: Refactor PciRootBridgeIo::enumerate() with tree-topology information #1830

Are you sure you want to change the base?

uefi: Refactor PciRootBridgeIo::enumerate() with tree-topology information #1830

Conversation

seijikun commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

phip1611 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seijikun Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

phip1611 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

seijikun commented Nov 21, 2025 •

edited

Loading

seijikun Nov 22, 2025 •

edited

Loading